-
Notifications
You must be signed in to change notification settings - Fork 225
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Support querying S2A Addresses from MDS #1400
base: main
Are you sure you want to change the base?
Conversation
0f96e86
to
ddac7aa
Compare
cc: @xmenxk |
Quality Gate passedIssues Measures |
@westarle, friendly ping :). Please review when you get a chance. |
return S2AConfig.createBuilder().build(); | ||
} | ||
|
||
for (int i = 0; i < MAX_MDS_PING_TRIES; i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only mentioned ExponentialBackOff
because that is used in one of the recent changes related to retry (https://github.com/googleapis/google-auth-library-java/pull/1452/files)
I suppose you can do similar, by using HttpRequest's built-in retry handlers and set request.setNumberOfRetries(OAuth2Utils.DEFAULT_NUMBER_OF_RETRIES);
? If you do not need ExponentialBackOff
, maybe something simple e.g.
// Set the number of retries
request.setNumberOfRetries(OAuth2Utils.DEFAULT_NUMBER_OF_RETRIES);
// Retry on specific status codes (you might want to adjust these)
request.setUnsuccessfulResponseHandler(
new HttpUnsuccessfulResponseHandler() {
@Override
public boolean handleResponse(
HttpRequest request, HttpResponse response, boolean supportsRetry)
throws IOException {
return RETRYABLE_STATUS_CODES.contains(response.getStatusCode());
}
});
About thread-safety, noticed HttpRequest is also marked as not safe here, likely because its member variable hold state information . But I don't see it a concern in this implementation, as executeAsync()
is not used and getS2AConfigFromMDS()
creates and executes a request each time.
@@ -143,6 +143,11 @@ private S2AConfig getS2AConfigFromMDS() { | |||
try { | |||
HttpResponse response = request.execute(); | |||
if (!response.isSuccessStatusCode()) { | |||
int statusCode = response.getStatusCode(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am leaning toward switching to HttpRequest's built-in retry handlers, logic like to customize the retry behavior based on return code can go into HttpUnsuccessfulResponseHandler
. See other comment for details.
|
||
@CanIgnoreReturnValue | ||
public Builder setPlaintextAddress(String plaintextAddress) { | ||
this.plaintextAddress = plaintextAddress; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add this explanation as javadoc comment here or to getS2AConfigFromMDS()
for future references?
try { | ||
request = transportFactory.create().createRequestFactory().buildGetRequest(genericUrl); | ||
request.setParser(parser); | ||
request.getHeaders().set(METADATA_FLAVOR, GOOGLE); | ||
request.setThrowExceptionOnExecuteError(false); | ||
request.setNumberOfRetries(OAuth2Utils.DEFAULT_NUMBER_OF_RETRIES); | ||
|
||
ExponentialBackOff backoff = | ||
new ExponentialBackOff.Builder() | ||
.setInitialIntervalMillis(OAuth2Utils.INITIAL_RETRY_INTERVAL_MILLIS) | ||
.setRandomizationFactor(OAuth2Utils.RETRY_RANDOMIZATION_FACTOR) | ||
.setMultiplier(OAuth2Utils.RETRY_MULTIPLIER) | ||
.build(); | ||
|
||
// Retry on 5xx status codes. | ||
request.setUnsuccessfulResponseHandler( | ||
new HttpBackOffUnsuccessfulResponseHandler(backoff) | ||
.setBackOffRequired( | ||
response -> RETRYABLE_STATUS_CODES.contains(response.getStatusCode()))); | ||
request.setIOExceptionHandler(new HttpBackOffIOExceptionHandler(backoff)); | ||
} catch (IOException e) { | ||
return S2AConfig.createBuilder().build(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
S2AConfig.createBuilder().build()
sets plainTextAddress and mtlsS2AAddress to ""
by default. Can we move this code in this try block into the one below (merging them)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in ed681f5
oauth2_http/javatests/com/google/auth/oauth2/MockMetadataServerTransport.java
Outdated
Show resolved
Hide resolved
// Create the JSON response | ||
GenericJson content = new GenericJson(); | ||
content.setFactory(OAuth2Utils.JSON_FACTORY); | ||
if (requestStatusCode < 400) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Can this be 200? Or are there other 2xx/ 3xx codes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah it can just be 200. We expect only 200 on successful responses. Done in 20825f7
private String plaintextS2AAddressJsonKey; | ||
|
||
private String plaintextS2AAddress; | ||
|
||
private String mtlsS2AAddressJsonKey; | ||
|
||
private String mtlsS2AAddress; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Can these values be stored as a map?
perhaps Map<String, String> s2aContentMap
(or a different name)? there would be one setter setS2AContentMap()
instead of 4?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 594df7b
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working with us to address the comments. I think this mostly looks good to me given the context.
Can you also do two additional things:
- Run
mvn fmt:format
on this PR? - Update the title to be
feat: {title}
? Something that describes the functionalityfeat: Support querying S2A Addresses from MDS
or whatever you think aptly describes it.
@lqiu96 , I've addressed your latest round of comments -- thanks for the feedback! Run mvn fmt:format on this PR?
Update the title to be feat: {title}? Something that describes the functionality feat: Support querying S2A Addresses from MDS or whatever you think aptly describes it.
|
transportFactory.transport.setS2AContentMap( | ||
"plaintextS2AAddressJsonKey", S2A.S2A_PLAINTEXT_ADDRESS_JSON_KEY); | ||
transportFactory.transport.setS2AContentMap("plaintextS2AAddress", S2A_PLAINTEXT_ADDRESS); | ||
transportFactory.transport.setS2AContentMap( | ||
"mtlsS2AAddressJsonKey", S2A.S2A_MTLS_ADDRESS_JSON_KEY); | ||
transportFactory.transport.setS2AContentMap("mtlsS2AAddress", S2A_MTLS_ADDRESS); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I was going to suggest something like:
For valid data:
transportFactory.transport.setS2AContentMap(
ImmutableMap.of(S2A.S2A_PLAINTEXT_ADDRESS_JSON_KEY, S2A_PLAINTEXT_ADDRESS,
S2A.S2A_MTLS_ADDRESS_JSON_KEY, S2A_MTLS_ADDRESS));
For invalid data:
transportFactory.transport.setS2AContentMap(
ImmutableMap.of(INVALID_JSON_KEY, S2A_PLAINTEXT_ADDRESS,
S2A.S2A_MTLS_ADDRESS_JSON_KEY, S2A_MTLS_ADDRESS));
And the content would be populated via iteration through tkey k/v's of the map. Something like:
if (requestStatusCode == 200) {
for (Map.Entry<String, String> entrySet: content.entrySet()) {
content.put(entrySet.getKey(), entrySet.getValue())
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is more succinct -- thanks. Done in 1e6c058
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Added a few nits. Please do let @zhumin8 to review and give an approval as well
return new Builder(); | ||
} | ||
|
||
public String getPlaintextAddress() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like Javadocs for all these public things, pointing to public docs for MDS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't have this documented in public MDS docs (e.g. https://cloud.google.com/compute/docs/metadata/predefined-metadata-keys). We do have an AIP: https://google.aip.dev/auth/4115 which discusses this autconfig endpoint and how it fits in the mTLS via S2A + bound tokens story. WDYT about 934679c?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added two minor comments below, otherwise LGTM.
OAuth2Utils.validateString( | ||
responseData, S2A_PLAINTEXT_ADDRESS_JSON_KEY, PARSE_ERROR_S2A); | ||
} catch (IOException ignore) { | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add comment on why these are ignored?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a comment in 6644d50. In general, if there is any error in this function, we ignore, and populate empty addresses in the S2AConfig
.
HttpResponse response = request.execute(); | ||
InputStream content = response.getContent(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe do try/catch block around these to be more specific about the exceptions catching? Nested try/catch blocks are a bit hard to read.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I think I was the one that suggested this, but am open to having the try-catch blocks be more specific or a different implementation.
I suggested just have one try-catch block since this code doesn't catch or return the error message back to the user. It just catches any IO error and returns an empty S2AConfig back. Unfortunately, this doesn't provide any useful feedback to the user. Given that this is how it behaves already in Go, I thought it would just be simpler to have a catch-all (one big try block) for this logic.
Ideally, we would have more specific try-catch blocks which could tell the user exactly what went wrong (i.e. failed to build the request because x, or the request failed because of y).
I believe there are two spots where we potentially would need to add try-catches:
- parseAs (Ah, thanks for the correction below)
- request.execute()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a few places in this block that throw an IOException
(the parseAs
and buildGetRequest
also throw IOException
), so I removed the current nested try/catch block structure and added two separate try/catch blocks around the lines that throw an exception. Because we are getting rid of the big block, I also moved around variable definitions closer to where they are used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lqiu96 , I think my changes and your comment happened at same time. Reading through your comment, the changes I just push match your suggestion. I removed the big try/catch and just did 2 smaller ones around building the request and executing it.
@zhumin8 , @lqiu96: I am fine with either option -- let me know if you prefer reverting to the 1 big/try catch block, or if you prefer the current state which has no nesting, but a few smaller try/catch blocks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am fine with splitting it up to reduce nesting.
Quality Gate passedIssues Measures |
Add utility to get S2A address from MDS MTLS autoconfiguration endpoint.
This utility will be used when creating mTLS channel using S2A Java Client, which takes S2A Address as input to create S2AChannelCredentials.
Parallel change in go: googleapis/google-api-go-client#1874
S2A Java client: grpc/grpc-java#11113